Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Audio] 페이지에 오디오 플레이어 연결 #122

Merged
merged 38 commits into from
Dec 4, 2024

Conversation

yuncheol-AHN
Copy link
Collaborator

@yuncheol-AHN yuncheol-AHN commented Dec 2, 2024

#️⃣ 연관된 이슈


⏰ 작업 시간

예상 시간 실제 걸린 시간
3.5 10+

📝 작업 내용

  • 페이지 생성에서 오디오 플레이어
  • 페이지 수정에서 오디오 플레이어
  • 페이지 읽기에서 오디오 플레이어

📸 스크린샷

ScreenRecording_12-04-2024.02-50-43_1.MP4

@yuncheol-AHN yuncheol-AHN self-assigned this Dec 2, 2024
@yuncheol-AHN yuncheol-AHN linked an issue Dec 2, 2024 that may be closed by this pull request
@yuncheol-AHN yuncheol-AHN marked this pull request as ready for review December 3, 2024 17:53
@yuncheol-AHN yuncheol-AHN changed the title [draft] Feature/audioplayer [Audio] 페이지에 오디오 플레이어 연결 Dec 3, 2024
@yuncheol-AHN yuncheol-AHN added ✨ Feature 기능 관련 작업 🎨 Design 에셋, 컴포넌트 작업 labels Dec 3, 2024
@yuncheol-AHN yuncheol-AHN added this to the 0.5 milestone Dec 3, 2024
Copy link
Collaborator

@Kyxxn Kyxxn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 !!!!!
로직 많이 겹쳐서 충돌 많이 났을텐데 고생하셨습니다 !!!!!!
밑에 수정할 부분들 코멘트 남겨뒀습니당

Comment on lines 15 to 62
nonisolated(unsafe) var audioPlayer: AVAudioPlayer?
var audioPlayState: AudioPlayState = .pause {
didSet {
switch audioPlayState {
case .play:
startTimer()
case .pause:
stopTimer()
}
}
}
var timer: Timer?

// MARK: - ViewComponent
let backgroundBorderView: UIView = {
let backgroundBorderView = UIView()
backgroundBorderView.backgroundColor = .baseBackground
backgroundBorderView.layer.borderWidth = 3
backgroundBorderView.layer.cornerRadius = 25
backgroundBorderView.layer.borderColor = UIColor.captionPlaceHolder.cgColor

return backgroundBorderView
}()
let audioProgressView: UIView = {
let backgroundView = UIView()
backgroundView.backgroundColor = .mhPink
// backgroundView.layer.borderWidth = 4
backgroundView.layer.cornerRadius = 21
// backgroundView.layer.borderColor = UIColor.baseBackground.cgColor

return backgroundView
}()
var progressViewWidthConstraint: NSLayoutConstraint?
var progressViewConstraints: [NSLayoutConstraint] = []
let audioStateButton: UIButton = {
let button = UIButton()
button.setImage(UIImage(systemName: "play.fill"), for: .normal)
return button
}()
let playImage = UIImage(systemName: "play.fill")
let pauseImage = UIImage(systemName: "pause.fill")
let audioStateImageView: UIImageView = {
let imageView = UIImageView()
imageView.image = UIImage(named: "audio_play")

return imageView
}()
let audioPlayTimeLabel: UILabel = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 여기 private 키워드만 붙여주시면 감사하겠습니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD😊

Comment on lines 41 to 43
// backgroundView.layer.borderWidth = 4
backgroundView.layer.cornerRadius = 21
// backgroundView.layer.borderColor = UIColor.baseBackground.cgColor
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 불필요한 주석도 제거하면 좋을 거 같아요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD😊

Comment on lines 7 to 8

final public class MHAudioPlayerView: UIView {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1
public 제거하고, extension에 있는 메소드도 Public 제거 해주시면 감사하겠습니다 !

P3
7번 줄 개행 제거해주시면 감사하겠습니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD😊

}

extension MHAudioPlayerView: AVAudioPlayerDelegate {
nonisolated public func audioPlayerDidFinishPlaying(_ player: AVAudioPlayer, successfully flag: Bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 public

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD😊

case .image:
MHPolaroidPhotoView()
case .video:
MHPolaroidPhotoView()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1
제 똥이긴 한데 여기 MHVideoView()로 바꿔주실 수 있나요 ??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD😊

Comment on lines -108 to +110
attachmentMetaData.forEach { location, description in
attachmentMetaData.forEach {
location,
description in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 이전 버전이 더 좋아보이는데
후자가 나으신가용 ? (단순 궁금증입니다)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD😊

Comment on lines 5 to 8
<key>UIFileSharingEnabled</key>
<true/>
<key>LSSupportsOpeningDocumentsInPlace</key>
<true/>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1
제거하기로 결정했습니다 !!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD😊

Copy link
Collaborator

@k2645 k2645 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오디오 만들어주시느라 정말 수고하셨습니다 .ᐟ.ᐟ 🙇🏻‍♀️

궁금한 사항들이랑 수정되면 좋을 것 같은 내용들 코멘트로 조금 남겨두었습니다 .ᐟ.ᐟ

audioRecorder?.isMeteringEnabled = true
print("URL: \(url)")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: print문은 지워주면 좋을 것 같습니당 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD😊

Comment on lines 243 to 253
audioButton.addAction(UIAction { [weak self] _ in
self?.input.send(.audioButtonTapped)
}, for: .touchUpInside)
}
private func addTappedEventToCancelButton() {
cancelButton.addAction(
UIAction { [weak self] _ in
self?.input.send(.recordCancelled)
},
for: .touchUpInside)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: 개행 컨벤션이 일정하지 않은데 통일시켜주면 좋을 것 같습니다 !

Comment on lines 275 to 281
AVAudioSession.sharedInstance().requestRecordPermission { granted in
Task { @MainActor in
if !granted {
self.present(alert, animated: true, completion: nil)
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: 이쪽 혹시 Test 해보셨을까요?? 아마 requestRecordPermission 후행 클로저에 Sendable 붙이지 않으면 앱이 런타임 시점에 죽을 것 같습니다 ~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AVAudioSession.sharedInstance().requestRecordPermission { @Sendable granted  in
    Task { @MainActor in
        if !granted {
            self.present(alert, animated: true, completion: nil)
        }
    }
}

이게 맞을까요? 테스트는 아직...

Copy link
Collaborator

@iceHood iceHood Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16.4시뮬로 테스트해봤었는데 잘 됐었습니다!

Comment on lines 225 to 235
func configureSource(with mediaDescription: MediaDescription, data: Data) {

}

func configureSource(with mediaDescription: MediaDescription, url: URL) {
MHLogger.debug("configure source \(url)")
audioPlayer = try? AVAudioPlayer(contentsOf: url)
guard let audioPlayer else { return }
audioPlayer.delegate = self
self.setTimeLabel(seconds: Int(audioPlayer.duration.rounded()))
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 해당 부분은 사용될 일은 없지만 만약을 위해 하단 url과 동일하게 메서드를 채워줘야할 것 같습니다 ! 약간 required init같은 느낌?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AVAduioPlayer Object 생성자 중 매개변수를 Data로 받는 부분이 없어서 조금 애매한데 Logger라도 찍는게 나을까요?

Comment on lines 183 to 188
if audioPlayer.isPlaying {
self?.updatePlayAudioProgress()
self?.setTimeLabel(seconds: Int(audioPlayer.currentTime))
} else {

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: else문엔 어떤 처리가 들어가야하나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제거했습니다!

Comment on lines 173 to 175
UIView.animate(withDuration: 0) {
self.layoutIfNeeded()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 애니메이션이 0초만에 일어나는 것이라면 애니메이션을 넣어야할 필요 있나요? 혹시 어떤 애니메이션때문에 해당 코드를 작성해주신 건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트해보니 필요 없을 거 같아요


private func updatePlayAudioProgress() {
guard let audioPlayer else { return }
let width = ceil(Float(audioPlayer.currentTime) / Float(audioPlayer.duration) * Float(299))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: 299라는 숫자는 무엇을 의미하는 숫자인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

audio media 길이입니다!

Comment on lines 159 to 163
if audioPlayer?.play() == false {
MHLogger.error("do not play")
} else {
MHLogger.debug("do play")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 해당 로직이 어떤 로직인지 잘 이해가 가지 않습니다.. 혹시 설명해주실 수 있으신가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오디오 플레이 테스트할 때 사용되었던 건데 지웠습니다.

private let output = PassthroughSubject<Output, Never>()
private var cancellables = Set<AnyCancellable>()
private var audioIsRecoding: Bool = false
private let completion: (MediaDescription?) -> Void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: completion 핸들러는 보통 VC에 있었던 것 같은데, ViewModel에 completion이 있게 된 경위?가 궁금합니다 !

Copy link
Collaborator

@iceHood iceHood Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VC에서 input(화면 닫히는 이벤트들)을 ViewModel에 보내면 ViewModel이 알아서 수행하도록 하고자 했습니다.
(VC로 옮겨도 상관 무)

Comment on lines 254 to 264
private func mediaViewFactory(type: MediaType) -> UIView & MediaAttachable {
switch type {
case .image:
MHPolaroidPhotoView()
case .video:
MHVideoView()
case .audio:
MHAudioPlayerView()
default:
MHPolaroidPhotoView()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: 해당 메서드를 생성한건 좋은데 사용되지 않는 것 같습니다 .ᐟ.ᐟ 위 코드들을 해당 메서드를 활용하도록 변경해주면 좋을 것 같네용 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

필요 없는 거 같아 지웠습니다.

@yuncheol-AHN yuncheol-AHN requested review from Kyxxn and k2645 December 3, 2024 19:21
Copy link
Collaborator

@Kyxxn Kyxxn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 〰️

Copy link
Collaborator

@k2645 k2645 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니당 ~.ᐟ.ᐟ

Copy link
Collaborator

@iceHood iceHood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 고생하셨습니다...ㅠㅠㅠㅠ

@yuncheol-AHN yuncheol-AHN merged commit 89f9cd0 into develop Dec 4, 2024
2 checks passed
@yuncheol-AHN yuncheol-AHN deleted the feature/audioplayer branch December 4, 2024 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎨 Design 에셋, 컴포넌트 작업 ✨ Feature 기능 관련 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

오디오 컨트롤
4 participants